-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Incorporate all node weights in additive blending #16279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incorporate all node weights in additive blending #16279
Conversation
|
@pcwalton @james7132 @mockersf I'm inclined to include this in 0.15, to avoid shipping weird behavior. |
pcwalton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, well motivated and seems like a straightforward bug fix. Agree with shipping as part of the release.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much less confusing behavior, and the changes are simple and clear.
# Objective
In the existing implementation, additive blending effectively treats the
node with least index specially by basically forcing its weight to be
`1.0` regardless of what its computed weight would be (based on the
weights in the `AnimationGraph` and `AnimationPlayer`).
Arguably this makes some amount of sense, because the "base" animation
is often one which was not authored to be used additively, meaning that
its sampled values are interpreted absolutely rather than as deltas.
However, this also leads to strange behavior with respect to animation
masks: if the "base" animation is masked out on some target, then the
next node is treated as the "base" animation, despite the fact that it
would normally be interpreted additively, and the weight of that
animation is thrown away as a result.
This is all kind of weird and revolves around special treatment (if the
behavior is even really intentional in the first place). From a
mathematical standpoint, there is nothing special about how the "base"
animation must be treated other than having a weight of 1.0 under an
`Add` node, which is something that the user can do without relying on
some bizarre corner-case behavior of the animation system — this is the
only present situation under which weights are discarded.
This PR changes this behavior so that the weight of every node is
incorporated. In other words, for an animation graph that looks like
this:
```text
┌───────────────┐
│Base clip ┼──┐
│ 0.5 │ │
└───────────────┘ │
┌───────────────┐ │ ┌───────────────┐ ┌────┐
│Additive clip 1┼──┼─►┤Additive blend ┼────►│Root│
│ 0.1 │ │ │ 1.0 │ └────┘
└───────────────┘ │ └───────────────┘
┌───────────────┐ │
│Additive clip 2┼──┘
│ 0.2 │
└───────────────┘
```
Previously, the result would have been
```text
base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
whereas now it would be
```text
0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
and in the scenario where `base_clip` is masked out:
```text
additive_clip_1 + 0.2 * additive_clip_2
```
vs.
```text
0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
## Solution
For background, the way that the additive blending procedure works is
something like this:
- During graph traversal, the node values and weights of the children
are pushed onto the evaluator `stack`. The traversal order guarantees
that the item with least node index will be on top.
- Once we reach the `Add` node itself, we start popping off the `stack`
and into the evaluator's `blend_register`, which is an accumulator
holding up to one weight-value pair:
- If the `blend_register` is empty, it is filled using data from the top
of the `stack`.
- Otherwise, the `blend_register` is combined with data popped from the
`stack` and updated.
In the example above, the additive blending steps would look like this
(with the pre-existing implementation):
1. The `blend_register` is empty, so we pop `(base_clip, 0.5)` from the
top of the `stack` and put it in. Now the value of the `blend_register`
is `(base_clip, 0.5)`.
2. The `blend_register` is non-empty: we pop `(additive_clip_1, 0.1)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1, 0.6)`
in the `blend_register` (the carried weight value goes unused).
3. The `blend_register` is non-empty: we pop `(additive_clip_2, 0.2)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1 + 0.2
* additive_clip_2, 0.8)` in the `blend_register`.
The solution in this PR changes step 1: the `base_clip` is multiplied by
its weight as it is added to the `blend_register` in the first place,
yielding `0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 *
additive_clip_2` as the final result.
### Note for reviewers
It might be tempting to look at the code, which contains a segment that
looks like this:
```rust
if additive {
current_value = A::blend(
[
BlendInput {
weight: 1.0, // <--
value: current_value,
additive: true,
},
BlendInput {
weight: weight_to_blend,
value: value_to_blend,
additive: true,
},
]
.into_iter(),
);
}
```
and conclude that the explicit value of `1.0` is responsible for
overwriting the weight of the base animation. This is incorrect.
Rather, this additive blend has to be written this way because it is
multiplying the *existing value in the blend register* by 1 (i.e. not
doing anything) before adding the next value to it. Changing this to
another quantity (e.g. the existing weight) would cause the value in the
blend register to be spuriously multiplied down.
## Testing
Tested on `animation_masks` example. Checked `morph_weights` example as
well.
## Migration Guide
I will write a migration guide later if this change is not included in
0.15.
# Objective
In the existing implementation, additive blending effectively treats the
node with least index specially by basically forcing its weight to be
`1.0` regardless of what its computed weight would be (based on the
weights in the `AnimationGraph` and `AnimationPlayer`).
Arguably this makes some amount of sense, because the "base" animation
is often one which was not authored to be used additively, meaning that
its sampled values are interpreted absolutely rather than as deltas.
However, this also leads to strange behavior with respect to animation
masks: if the "base" animation is masked out on some target, then the
next node is treated as the "base" animation, despite the fact that it
would normally be interpreted additively, and the weight of that
animation is thrown away as a result.
This is all kind of weird and revolves around special treatment (if the
behavior is even really intentional in the first place). From a
mathematical standpoint, there is nothing special about how the "base"
animation must be treated other than having a weight of 1.0 under an
`Add` node, which is something that the user can do without relying on
some bizarre corner-case behavior of the animation system — this is the
only present situation under which weights are discarded.
This PR changes this behavior so that the weight of every node is
incorporated. In other words, for an animation graph that looks like
this:
```text
┌───────────────┐
│Base clip ┼──┐
│ 0.5 │ │
└───────────────┘ │
┌───────────────┐ │ ┌───────────────┐ ┌────┐
│Additive clip 1┼──┼─►┤Additive blend ┼────►│Root│
│ 0.1 │ │ │ 1.0 │ └────┘
└───────────────┘ │ └───────────────┘
┌───────────────┐ │
│Additive clip 2┼──┘
│ 0.2 │
└───────────────┘
```
Previously, the result would have been
```text
base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
whereas now it would be
```text
0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
and in the scenario where `base_clip` is masked out:
```text
additive_clip_1 + 0.2 * additive_clip_2
```
vs.
```text
0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
## Solution
For background, the way that the additive blending procedure works is
something like this:
- During graph traversal, the node values and weights of the children
are pushed onto the evaluator `stack`. The traversal order guarantees
that the item with least node index will be on top.
- Once we reach the `Add` node itself, we start popping off the `stack`
and into the evaluator's `blend_register`, which is an accumulator
holding up to one weight-value pair:
- If the `blend_register` is empty, it is filled using data from the top
of the `stack`.
- Otherwise, the `blend_register` is combined with data popped from the
`stack` and updated.
In the example above, the additive blending steps would look like this
(with the pre-existing implementation):
1. The `blend_register` is empty, so we pop `(base_clip, 0.5)` from the
top of the `stack` and put it in. Now the value of the `blend_register`
is `(base_clip, 0.5)`.
2. The `blend_register` is non-empty: we pop `(additive_clip_1, 0.1)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1, 0.6)`
in the `blend_register` (the carried weight value goes unused).
3. The `blend_register` is non-empty: we pop `(additive_clip_2, 0.2)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1 + 0.2
* additive_clip_2, 0.8)` in the `blend_register`.
The solution in this PR changes step 1: the `base_clip` is multiplied by
its weight as it is added to the `blend_register` in the first place,
yielding `0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 *
additive_clip_2` as the final result.
### Note for reviewers
It might be tempting to look at the code, which contains a segment that
looks like this:
```rust
if additive {
current_value = A::blend(
[
BlendInput {
weight: 1.0, // <--
value: current_value,
additive: true,
},
BlendInput {
weight: weight_to_blend,
value: value_to_blend,
additive: true,
},
]
.into_iter(),
);
}
```
and conclude that the explicit value of `1.0` is responsible for
overwriting the weight of the base animation. This is incorrect.
Rather, this additive blend has to be written this way because it is
multiplying the *existing value in the blend register* by 1 (i.e. not
doing anything) before adding the next value to it. Changing this to
another quantity (e.g. the existing weight) would cause the value in the
blend register to be spuriously multiplied down.
## Testing
Tested on `animation_masks` example. Checked `morph_weights` example as
well.
## Migration Guide
I will write a migration guide later if this change is not included in
0.15.
# Objective
In the existing implementation, additive blending effectively treats the
node with least index specially by basically forcing its weight to be
`1.0` regardless of what its computed weight would be (based on the
weights in the `AnimationGraph` and `AnimationPlayer`).
Arguably this makes some amount of sense, because the "base" animation
is often one which was not authored to be used additively, meaning that
its sampled values are interpreted absolutely rather than as deltas.
However, this also leads to strange behavior with respect to animation
masks: if the "base" animation is masked out on some target, then the
next node is treated as the "base" animation, despite the fact that it
would normally be interpreted additively, and the weight of that
animation is thrown away as a result.
This is all kind of weird and revolves around special treatment (if the
behavior is even really intentional in the first place). From a
mathematical standpoint, there is nothing special about how the "base"
animation must be treated other than having a weight of 1.0 under an
`Add` node, which is something that the user can do without relying on
some bizarre corner-case behavior of the animation system — this is the
only present situation under which weights are discarded.
This PR changes this behavior so that the weight of every node is
incorporated. In other words, for an animation graph that looks like
this:
```text
┌───────────────┐
│Base clip ┼──┐
│ 0.5 │ │
└───────────────┘ │
┌───────────────┐ │ ┌───────────────┐ ┌────┐
│Additive clip 1┼──┼─►┤Additive blend ┼────►│Root│
│ 0.1 │ │ │ 1.0 │ └────┘
└───────────────┘ │ └───────────────┘
┌───────────────┐ │
│Additive clip 2┼──┘
│ 0.2 │
└───────────────┘
```
Previously, the result would have been
```text
base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
whereas now it would be
```text
0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
and in the scenario where `base_clip` is masked out:
```text
additive_clip_1 + 0.2 * additive_clip_2
```
vs.
```text
0.1 * additive_clip_1 + 0.2 * additive_clip_2
```
## Solution
For background, the way that the additive blending procedure works is
something like this:
- During graph traversal, the node values and weights of the children
are pushed onto the evaluator `stack`. The traversal order guarantees
that the item with least node index will be on top.
- Once we reach the `Add` node itself, we start popping off the `stack`
and into the evaluator's `blend_register`, which is an accumulator
holding up to one weight-value pair:
- If the `blend_register` is empty, it is filled using data from the top
of the `stack`.
- Otherwise, the `blend_register` is combined with data popped from the
`stack` and updated.
In the example above, the additive blending steps would look like this
(with the pre-existing implementation):
1. The `blend_register` is empty, so we pop `(base_clip, 0.5)` from the
top of the `stack` and put it in. Now the value of the `blend_register`
is `(base_clip, 0.5)`.
2. The `blend_register` is non-empty: we pop `(additive_clip_1, 0.1)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1, 0.6)`
in the `blend_register` (the carried weight value goes unused).
3. The `blend_register` is non-empty: we pop `(additive_clip_2, 0.2)`
from the top of the `stack` and combine it additively with the value in
the `blend_register`, forming `(base_clip + 0.1 * additive_clip_1 + 0.2
* additive_clip_2, 0.8)` in the `blend_register`.
The solution in this PR changes step 1: the `base_clip` is multiplied by
its weight as it is added to the `blend_register` in the first place,
yielding `0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 *
additive_clip_2` as the final result.
### Note for reviewers
It might be tempting to look at the code, which contains a segment that
looks like this:
```rust
if additive {
current_value = A::blend(
[
BlendInput {
weight: 1.0, // <--
value: current_value,
additive: true,
},
BlendInput {
weight: weight_to_blend,
value: value_to_blend,
additive: true,
},
]
.into_iter(),
);
}
```
and conclude that the explicit value of `1.0` is responsible for
overwriting the weight of the base animation. This is incorrect.
Rather, this additive blend has to be written this way because it is
multiplying the *existing value in the blend register* by 1 (i.e. not
doing anything) before adding the next value to it. Changing this to
another quantity (e.g. the existing weight) would cause the value in the
blend register to be spuriously multiplied down.
## Testing
Tested on `animation_masks` example. Checked `morph_weights` example as
well.
## Migration Guide
I will write a migration guide later if this change is not included in
0.15.
Objective
In the existing implementation, additive blending effectively treats the node with least index specially by basically forcing its weight to be
1.0regardless of what its computed weight would be (based on the weights in theAnimationGraphandAnimationPlayer).Arguably this makes some amount of sense, because the "base" animation is often one which was not authored to be used additively, meaning that its sampled values are interpreted absolutely rather than as deltas. However, this also leads to strange behavior with respect to animation masks: if the "base" animation is masked out on some target, then the next node is treated as the "base" animation, despite the fact that it would normally be interpreted additively, and the weight of that animation is thrown away as a result.
This is all kind of weird and revolves around special treatment (if the behavior is even really intentional in the first place). From a mathematical standpoint, there is nothing special about how the "base" animation must be treated other than having a weight of 1.0 under an
Addnode, which is something that the user can do without relying on some bizarre corner-case behavior of the animation system — this is the only present situation under which weights are discarded.This PR changes this behavior so that the weight of every node is incorporated. In other words, for an animation graph that looks like this:
Previously, the result would have been
whereas now it would be
and in the scenario where
base_clipis masked out:vs.
Solution
For background, the way that the additive blending procedure works is something like this:
stack. The traversal order guarantees that the item with least node index will be on top.Addnode itself, we start popping off thestackand into the evaluator'sblend_register, which is an accumulator holding up to one weight-value pair:blend_registeris empty, it is filled using data from the top of thestack.blend_registeris combined with data popped from thestackand updated.In the example above, the additive blending steps would look like this (with the pre-existing implementation):
blend_registeris empty, so we pop(base_clip, 0.5)from the top of thestackand put it in. Now the value of theblend_registeris(base_clip, 0.5).blend_registeris non-empty: we pop(additive_clip_1, 0.1)from the top of thestackand combine it additively with the value in theblend_register, forming(base_clip + 0.1 * additive_clip_1, 0.6)in theblend_register(the carried weight value goes unused).blend_registeris non-empty: we pop(additive_clip_2, 0.2)from the top of thestackand combine it additively with the value in theblend_register, forming(base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2, 0.8)in theblend_register.The solution in this PR changes step 1: the
base_clipis multiplied by its weight as it is added to theblend_registerin the first place, yielding0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2as the final result.Note for reviewers
It might be tempting to look at the code, which contains a segment that looks like this:
and conclude that the explicit value of
1.0is responsible for overwriting the weight of the base animation. This is incorrect.Rather, this additive blend has to be written this way because it is multiplying the existing value in the blend register by 1 (i.e. not doing anything) before adding the next value to it. Changing this to another quantity (e.g. the existing weight) would cause the value in the blend register to be spuriously multiplied down.
Testing
Tested on
animation_masksexample. Checkedmorph_weightsexample as well.Migration Guide
I will write a migration guide later if this change is not included in 0.15.